Skip to content

Implemented one-off query support for TypeScript SDK #2960

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

egormanga
Copy link

Description of Changes

Was necessary (at least) because of #1599.

API and ABI breaking changes

None(?)

Needs docs, that's for certain.

Expected complexity level and risk

4:

Doesn't seem to break anything, but who knows. I'm not sure about this one.

Also, performance impact? It should only get better, but again I have no info on that.

Testing

  • Manual testing
  • Use case fit
  • Unit tests

Copy link
Contributor

@gefjon gefjon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks so much for your contribution! This is a feature that we're very interested in merging. As discussed in comments below, we'd prefer to have the TypeScript client SDK expose the same interface for remote queries as the C# client SDK. That interface is:

Each TableHandle returned by the methods on ctx.db has a method remoteQuery. That method looks roughly like:

class TableHandleImpl<Row> {
  async remoteQuery(filters: string): Row[] {
    const tableName = this.tableName();
    const fullQuery = `SELECT ${tableName}.* FROM {tableName} ${filters}`;
    // Construct a promise, register it in the `DbConnection`, send a `OneOffQuery` message, return the promise.
    // When the `DbConnection` receives the response, it parses the rows and fulfills the promise with them.
  }
}

Client code might invoke this like:

async getPlayersInFaction(ctx, factionId): Player[] {
  const interestingPlayers = await ctx.db.player().remoteQuery(`WHERE faction = ${factionId}`);
  return interestingPlayers;
}

Comment on lines 274 to 276
queryBuilder = (): QueryBuilderImpl => {
return new QueryBuilderImpl(this);
};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The API we use in the C# SDK for running remote queries has them as methods of the TableHandle, rather than on the DbContext. This provides some amount of type safety, as the method inserts the SELECT clause into the query and also encodes the TypeScript type of the rows returned.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, we use the phrase "remote query" to describe this operation in the user-facing API. This emphasizes that this is a query that bypasses the client cache and goes directly to the remote database.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At the same time, formatting SELECTs restricts the provided functionality to querying only. What if a client needs to execute something else, e.g. INSERT, perhaps with the owner's identity?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, there I discovered (in #2968) that SpacetimeDB does currently not support anything other than SELECT even in one-shot queries.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct, we have no interest in generalizing one-off queries to support SQL DML operations like INSERT, DELETE, UPDATE, &c.

Comment on lines 10 to 51
export class QueryBuilderImpl<
DBView = any,
Reducers = any,
SetReducerFlags = any,
> {
#onResolved?: (
ctx: QueryEventContextInterface<DBView, Reducers, SetReducerFlags>
) => void = undefined;
#onError?: (
ctx: ErrorContextInterface<DBView, Reducers, SetReducerFlags>
) => void = undefined;
constructor(
private db: DbConnectionImpl<DBView, Reducers, SetReducerFlags>
) {}

onResolved(
cb: (
ctx: QueryEventContextInterface<DBView, Reducers, SetReducerFlags>
) => void
): QueryBuilderImpl<DBView, Reducers, SetReducerFlags> {
this.#onResolved = cb;
return this;
}

onError(
cb: (ctx: ErrorContextInterface<DBView, Reducers, SetReducerFlags>) => void
): QueryBuilderImpl<DBView, Reducers, SetReducerFlags> {
this.#onError = cb;
return this;
}

query(
query_sql: string
): QueryHandleImpl<DBView, Reducers, SetReducerFlags> {
return new QueryHandleImpl(
this.db,
query_sql,
this.#onResolved,
this.#onError
);
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remote queries are an odd and mismatched part of our API. Rather than using a builder and callbacks, they are async methods which directly return the queried rows. What you've designed here would be better, in that it more closely matches the way we construct subscriptions. Unfortunately, though, the async version is a part of the C# client SDK's stable API, and we are committed to continuing to support it. We're also committed to minimizing differences between the SDKs, which means that TypeScript should expose the same interface.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be perfectly honest, async remote queries seem to be a more intuitive and straightforward solution, even though they differ from the subscriptions. Being able to use queries anywhere in the code without registering a callback is pretty convenient.

I see your appreciation and in that regard will surely replace the current implementation with a table method. On the other hand, might it be useful to keep both, e.g. even providing the builder version across all implementations?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The main showstopper for your proposed approach is the fact that the entire SDK is currently callback-based, thus rendering it pretty impossible to implement an async querying mechanism in a useful fashion.

For instance, if I need to fire a query inside a subscription callback, I won't be able to await for it inside that block. Since I'd need to wrap it in a Promise anyway, similar to the current API, why bother implementing this at all?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the other hand, might it be useful to keep both, e.g. even providing the builder version across all implementations?

We'd need a more involved design process in order to make this happen. Going through such a design process with someone external to Clockwork Labs poses a number of problems, and seems unlikely to be pleasant for anyone involved.

if I need to fire a query inside a subscription callback, I won't be able to await for it inside that block.

Correct, you'll instead do ctx.db.myTable().remoteQuery("WHERE whatever").then(rows => processRows(rows));.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, you may design the thing as you wish (or, may not at all). I'll look into the possibility of refactoring the implementation to not depend on this custom solution (and it's clearly possible, but however be the cleanest wins).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The builder showed to be more convenient even in the internal code, so I'm suggesting keeping it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't seem possible inside loops, though.

I don't understand why being inside a loop would be in any way different.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Say you need to fetch some relation for each row (basically, .map() them) and do something with the resulting array. You do a query, .then() on it, and inside that you iterate over the rows. On each iteration, you issue another query, and that's where you only can await it and then use Promise.all() on the whole .map(), otherwise it's a callback inside a loop and that's not possible without refactoring.

Example for my API (assume inside a promise):

ctx.subscriptionBuilder()
    .onApplied(ctx => {
        ctx.queryBuilder()
            .onResolved((ctx, tables) => {
                const messages = tables.get('message');
                resolve(messages?.iter().map(message => {
                    const user = ctx.db.user.id.find(message.authorId);
                    return {user, message};
                });
            })
            .query(`SELECT * FROM message;`);
    })
    .subscribe(`SELECT * FROM user;`);

Examples for your API:

ctx.subscriptionBuilder()
    .onApplied(ctx => {
        const messages = await ctx.db.message.remoteQuery(`SELECT * FROM message;`);
        //               ^^^^^ error: await inside non-async function
        resolve();
    })
    .subscribe(`SELECT * FROM user;`);

ctx.subscriptionBuilder()
    .onApplied(async ctx => {
        //     ^^^^^ sure possible, but who will ever await it?
        const messages = await ctx.db.message.remoteQuery();
        resolve();  // will never get called
    })
    .subscribe();

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your second example:

ctx.subscriptionBuilder()
  .onApplied(ctx => {
    ctx.db.message.remoteQuery("WHERE foo = bar")
      .then(messages => {
        resolve(doStuffWith(messages));
      });
  )
  .subscribe(`SELECT * FROM user;)`;

Your first example:

ctx.subscriptionBuilder()
  .onApplied(ctx => {
    ctx.db.message.remoteQuery("")
      .then(messages => {
        resolve(messages.iter().map(message => {
          const user = ctx.db.user.id.find(message.authorId);
          return { user, message };
        });
      });
  .subscribe(`SELECT * FROM user;`);

Keep in mind that in JS, promises are always run by the executor, and async/await is syntax sugar over the then callbacks. This is unlike e.g. Rust, where promises famously do nothing unless awaited.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, you convinced me, thank you. Let me make the API private in a sec.

@egormanga
Copy link
Author

It turned out to be a really convenient implementation of an async ctx.table.remoteQuery() using the builder I already implemented. Done just that.

@egormanga egormanga requested a review from gefjon July 22, 2025 02:46
Copy link
Contributor

@gefjon gefjon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer not to expose your query builder as a new public user-facing API. Even if it's not documented, people will inevitably stumble upon it and use it in their code. This will have the twin problems of exposing them to errors like the one you saw in #2968 , and forcing us to maintain the new API in a compatible way in order to preserve their code. To avoid this, I recommend moving it from a class method to a free function, and making sure said free function is not exported from the package.

this.#sendMessage(
ClientMessage.OneOffQuery({
queryString: querySql,
messageId: new Uint8Array([queryId]),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will be a problem after 255 queries. Using a 4 byte integer would probably be safe enough, but we would need to convert it to bytes with something like:

const queryId = this.#getNextQueryId();
const buffer = new ArrayBuffer(4);
const view = new DataView(buffer);
view.setUint32(0, queryId, true); // true = little-endian

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, thanks for spotting it! I added a single-byte counter as a placeholder and totally forgot to widen it.

);
for (const callbacks of tables.values())
for (const callback of callbacks)
callback.cb();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we want to call the row-level callbacks for one-off queries, since any rows that are inserted here will never have corresponding delete callbacks.

Since we aren't calling those, we don't need to have the #applyTableState function. We can just get a list of rows for each table.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree we can remove the callbacks. But applyTableState() processes the insertion events from the response, isn't it necessary to compute the rows?

@egormanga egormanga requested a review from jsdt August 11, 2025 13:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants